Skip to content

Coalesce duplicate BasicExecutor task wake-ups - #669

Open
Dylan-Gallagher wants to merge 1 commit into
ros2-rust:mainfrom
Dylan-Gallagher:fix/basic-executor-idle-spin
Open

Coalesce duplicate BasicExecutor task wake-ups#669
Dylan-Gallagher wants to merge 1 commit into
ros2-rust:mainfrom
Dylan-Gallagher:fix/basic-executor-idle-spin

Conversation

@Dylan-Gallagher

Copy link
Copy Markdown

Summary

  • track whether a BasicExecutorRuntime task already has a ready-queue entry
  • coalesce repeated wake-ups into one pending poll
  • preserve wake-ups delivered while the task is being polled
  • add regressions for burst wake-ups and wake-during-poll ordering

Context

The profile in #666 points at the task send/receive/waker path. The blocking
receiver does sleep when the ready queue is empty in a minimal idle executor,
but Task::wake_by_ref currently enqueues the same task on every notification.
A burst of notifications can therefore keep redundant task entries in the
queue and cause repeated polls after the future has already observed the latest
state.

Rust's waker contract allows multiple wake-ups before the next poll to be
coalesced. This change uses an atomic queued flag to allow only one outstanding
ready entry per task. The flag is cleared before Future::poll, which ensures
that a wake delivered during the poll still schedules another poll.

I could not reproduce the issue's exact 28% CPU result without the reporter's
full async service workload, so this PR makes the narrower claim: it removes a
deterministically reproduced queue-amplification mechanism in the profiled hot
path. A two-second minimal idle spin was already near zero CPU before the
change.

Testing

  • cargo test -F default,serde executor::basic_executor::tests -- --test-threads=1
    • 2 passed
  • cargo test -F default,serde -- --test-threads=1
    • 121 unit tests and 51 doc tests passed
  • colcon build --packages-select rclrs
    • passed in an isolated ROS 2 Jazzy container workspace
  • cargo fmt --all -- --check
  • rustfmt +nightly --edition 2021 --check rclrs/src/executor/basic_executor.rs
  • git diff --check

The burst-wake regression demonstrates that 1,000 wake notifications result in
one outstanding ready entry. The second regression verifies that a synchronous
self-wake during the first poll still produces the required second poll.

Relates to #666.

@azerupi

azerupi commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Hey @Dylan-Gallagher, thank you for the PR!

I looked into #666 and with the additional example provided by the reported I was able to reproduce and find out the cause. You can check out the issue for the explanation. As it stands, this PR would not solve the issue (I have tested), it's not really something we can solve easily in rclrs because it is a tokio behavior.

@espressolee espressolee left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head a0a85afb6159486b71450c130ef40c34fe53a707 against exact base 7ec55378e123df42f3671751161b9fbd046c4214. Current main is now 4606ed41a212fb6c65ab01d1e79ddabde4e7cb05, but basic_executor.rs is unchanged across that base drift and the merge tree is clean.

I agree with the existing discussion that this does not solve #666's Tokio cooperative-budget loop. I modeled the submitted Task/ArcWake/std::mpsc state machine independently under the project's Rust 1.85 MSRV and checked the narrower queue-coalescing claim:

  • 1,000 baseline burst wakes produce 1,000 queue entries; this head produces 1
  • 16 threads × 1,000 wakes still produce one outstanding entry
  • a cross-thread wake delivered while Future::poll is blocked schedules the required second poll
  • a synchronous wake immediately before Poll::Ready leaves one harmless no-op entry, which drains cleanly
  • a stale waker remains coalesced after completion
  • receiver disconnect resets queued after the failed send
  • an 8-thread, 16,000-generation wake storm eventually observes the final generation without a lost wake

The seven hostile tests passed 100 consecutive runs (700/700 total executions), with Rust 1.85 fmt and clippy -D warnings clean for the isolated model. The exact-head CI is green on Humble, Jazzy, Kilted, Rolling, and Windows; the stable Linux log reports 121 unit tests and 51 doc tests passing on each ROS distribution.

The important high-level limit is also deterministic: a future that self-wakes once on every poll and returns Pending still executes 1,000/1,000 polls with the ready queue remaining non-empty. Coalescing cannot affect the one-wake-per-poll Tokio coop-budget behavior described in #666.

I found no correctness blocker in the narrow queue-entry invariant. The remaining decision is whether the extra atomic on every wake is justified by a real burst-wake workload independent of #666. A representative benchmark or queue-depth/poll-count measurement would establish that value; otherwise this is a correct but currently unquantified optimization. The upstream regressions are synchronous, so a cross-thread wake-during-poll test would also strengthen the concurrency contract.

Local limitation: the repository's documentation-only ROS shim does not provide the generated message types needed to compile the full rclrs test target, and the local Docker daemon was unavailable. I therefore rely on the exact-head remote ROS matrix for full-crate qualification and keep the local claim bounded to the changed task state machine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants